radicle-surf: Repository.blob_at() to retrieve a blob using its oid.#124
radicle-surf: Repository.blob_at() to retrieve a blob using its oid.#124keepsimple1 wants to merge 1 commit intomainfrom
Conversation
Signed-off-by: Han Xu <keepsimple@gmail.com>
3f174d2 to
25add2b
Compare
| /// Retrieves the blob with `oid` in `commit`. | ||
| pub fn blob_at<'a, C: ToCommit>( | ||
| &'a self, | ||
| commit: C, |
There was a problem hiding this comment.
The input commit is not required if we only wanted to retrieve the blob content. It is here as we also need to retrieve the Blob.commit that created the blob (oid).
There was a problem hiding this comment.
I wasn't expecting to have the commit as a parameter, so what if we changed the result type to just BlobRef<'a>?
There was a problem hiding this comment.
I thought about it, and I chose to have the commit parameter for a couple of reasons:
-
To be consistent in the API (similar to
blob()), including the return typeBlob. The intent ofBlob.commitwas helping the caller to identify thelast_commiteasily without having to do another HTTP call. That is the same for bothblob()andblob_at(). -
From the zulip discussion linked in Description, the caller would (most likely) have the commit info available.
There was a problem hiding this comment.
From the zulip discussion linked in Description, the caller would (most likely) have the commit info available.
My reading of the discussion was that we should be able to get a blob without the commit[0]. So if something doesn't know about a commit but does have a blob id, then it can just retrieve the blob content.
There was a problem hiding this comment.
I was referring to the original request message.
Yeah, like we discussed so far, there are two options that I can see:
a) Get the blob content only (input: Oid). If the caller wanted to find the commit that created the blob, they make another HTTP call. This is not consistent with existing blob() method.
b) Get the blob content and the commit that created the blob. (input: Oid and a Commit that has the blob. ) . Both info are retrieved in one call. This is consistent with existing blob() method.
For the blob content part, b) does more involved work than a), because it also retrieves the commit.
It's a trade-off. My preference is to go with option b) based on the reasons I mentioned earlier. And if we did want to go with a), I would suggest to re-visit Blob struct and see if it makes sense to separate Blob.commit out, and then change blob() to be consistent with blob_at().
There was a problem hiding this comment.
I was referring to the original request message.
Sure and within that thread, I mentioned how I imagined the URL being '.../blob/<oid>' :)
I don't mind going down this route, but I don't understand why we have to go fetch the commit that created the blob. Can you explain why that's necessary?
I'd expect that we just check the Blob's Oid exists with the Commit's tree, either via diff or using a tree walk.
There was a problem hiding this comment.
I don't mind going down this route, but I don't understand why we have to go fetch the commit that created the blob. Can you explain why that's necessary?
It's because this existing Blob struct and its API:
radicle-git/radicle-surf/src/blob.rs
Lines 52 to 54 in 6de53f7
This is motived by a discussion on zulip.